Improve rendering of javadoc content#1981
Conversation
Render all inline tags in the same way as the standard doclet. Some code simplifications.
7cc1e9c to
74050ec
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Good improvement overall — replacing the fragile regex-based Javadoc tag post-processing with a proper SimpleDocTreeVisitor is the right approach. The handling of {@value}, {@systemProperty}, HTML elements, and entities is well done.
Bug — {@code} / {@literal} rendering swapped:
In visitLiteral (line 502), the condition node.getKind() == DocTree.Kind.LITERAL routes {@literal} through renderAsCode() and {@code} through escapeHtml(). Per the Javadoc spec:
{@code text}(Kind.CODE) should render as<code>text</code>{@literal text}(Kind.LITERAL) should render as plain escaped text
The condition should check DocTree.Kind.CODE instead. The sibling visitLink method in the same file demonstrates the correct pattern: LINK_PLAIN → escapeHtml(), LINK → renderAsCode().
The test at line 84 validates the swapped behavior — the expected string shows plain text for {@code} and <code> tags for {@literal}, which matches the buggy code but contradicts the spec.
Suggested fix:
if (node.getKind() == DocTree.Kind.CODE) {
return renderAsCode(node.getBody().getBody());
} else {
return escapeHtml(node.getBody().getBody());
}Minor: renderContent Javadoc has @param tags without descriptions.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The refactoring from regex-based cleanseTags/cleanseJavadoc post-processing to a proper SimpleDocTreeVisitor is a great improvement — much more robust. The added support for {@value}, {@systemProperty}, HTML elements, and entities is welcome, and the cleanup of nvl to Objects.toString is a nice simplification.
However, visitLiteral has the {@code} and {@literal} handling swapped. Details below.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
|
|
||
| @Override | ||
| public String visitLiteral(LiteralTree node, Void p) { | ||
| if (node.getKind() == DocTree.Kind.LITERAL) { |
There was a problem hiding this comment.
The condition here is inverted. DocTree.Kind.CODE corresponds to the {@code} tag (monospace, wrapped in <code>) while DocTree.Kind.LITERAL corresponds to the {@literal} tag (plain text, HTML-escaped, no monospace). The current code wraps {@literal} content in <code> and leaves {@code} content unwrapped — the opposite of what the standard doclet does.
Notably, visitLink (line 497) gets this right: it maps LINK (code-font semantics) to renderAsCode and LINK_PLAIN (plain-text semantics) to escapeHtml. The same pattern should apply here.
| if (node.getKind() == DocTree.Kind.LITERAL) { | |
| public String visitLiteral(LiteralTree node, Void p) { | |
| if (node.getKind() == DocTree.Kind.CODE) { | |
| return renderAsCode(node.getBody().getBody()); | |
| } else { | |
| return escapeHtml(node.getBody().getBody()); | |
| } | |
| } |
| assertEquals("", string.get("since"), "no @since expected"); | ||
| assertEquals("Yes", string.get("supportRepoIdSuffix")); | ||
| assertEquals( | ||
| "A string value with some inline tags. Value <code>\"hello\"</code> is the default. <code>some.property</code> is used. This text is code. <code>This text is literal.</code> <code>java.lang.String</code> is the type.", |
There was a problem hiding this comment.
The expected string here reflects the inverted behavior — it expects {@code This text is code.} to render without a <code> wrapper and {@literal This text is literal.} to render with one. Once the condition in visitLiteral is fixed, this expectation should be updated to match:
| "A string value with some inline tags. Value <code>\"hello\"</code> is the default. <code>some.property</code> is used. This text is code. <code>This text is literal.</code> <code>java.lang.String</code> is the type.", | |
| "A string value with some inline tags. Value <code>\"hello\"</code> is the default. <code>some.property</code> is used. <code>This text is code.</code> This text is literal. <code>java.lang.String</code> is the type.", |
Add Javadoc for renderContent(...)
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Solid refactoring that replaces fragile regex-based Javadoc tag post-processing with a proper SimpleDocTreeVisitor. The {@code}/{@literal} bug flagged by prior reviews was correctly fixed in commit 0c4adcb4.
Minor observations (non-blocking):
escapeHtmlescapes&,<,>but not"— if an attribute value ever contained the same quote character used as delimiter, the output HTML would be malformed. Extremely unlikely for Javadoc content.visitValuefor bare{@value}(no reference) produces<code></code>instead of the enclosing field's constant value per spec. Pre-existing limitation, not a regression.- Whitespace normalization (
replaceAll("\\s+", " ")) in thetrim=truepath collapses whitespace inside<code>spans. Documented limitation.
Strengths:
- The old
cleanseTagsregex\{@\w\w\w\w (.+?)\}could only match 4-letter tag names, silently leaving{@literal},{@value},{@systemProperty}unprocessed. The visitor approach handles all standard inline tags. - The
cleanseJavadocline-stripping was redundant sincegetFullBody()already excludes block tags. nvl→Objects.toStringmigration uses standard library.- Test coverage exercises
{@value},{@systemProperty},{@code},{@literal}, and{@link}inline tags.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
2 participants
Render all inline tags in the same way as the standard doclet. Some code simplifications.
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.